feat(cli): enhance ov --version to show both CLI and server versions#835
Closed
ZaynJarvis wants to merge 2 commits intomainfrom
Closed
feat(cli): enhance ov --version to show both CLI and server versions#835ZaynJarvis wants to merge 2 commits intomainfrom
ZaynJarvis wants to merge 2 commits intomainfrom
Conversation
- Display CLI version with "CLI version:" label for clarity - Fetch server version from /health endpoint and display it - Handle server unavailable case with helpful config path hint - Show environment-aware config file path for troubleshooting - Prevents user confusion about CLI vs server version mismatch
|
Failed to generate code suggestions for PR |
Override clap's built-in --version/-V to also fetch and display the server version from /health endpoint. Shows a config path hint when the server is unreachable, preventing CLI vs server version confusion.
qin-ctx
requested changes
Mar 21, 2026
Collaborator
qin-ctx
left a comment
qin-ctx
left a comment
There was a problem hiding this comment.
One blocking issue found — misleading error message when config loading fails in the --version handler.
| } | ||
| } | ||
| } else { | ||
| println!("Server: not found"); |
Collaborator
There was a problem hiding this comment.
[Bug] (blocking) When CliContext::new() fails (most commonly because the config file is missing or malformed), this branch prints "Server: not found", which is misleading — the actual problem is that the config couldn't be loaded, so the CLI doesn't even know the server's address.
Additionally, unlike the Err(_) branch on line 538 which shows the config path hint, this branch omits it — precisely in the scenario (missing config) where the hint is most needed.
Suggested fix:
} else {
println!("Server: config not loaded");
let config_path = if let Ok(env_path) = std::env::var("OPENVIKING_CLI_CONFIG_FILE") {
env_path
} else {
match config::default_config_path() {
Ok(path) => path.to_string_lossy().to_string(),
Err(_) => "~/.openviking/ovcli.conf".to_string(),
}
};
println!("Check ovcli.conf: {}", config_path);
}
Collaborator
Author
|
close for other fix #869 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
CLI version:label/healthendpointChanges
Test plan
ov --versionwith server running — should show bothCLI version:andServer version:ov --versionwith server stopped — should showServer: not foundandCheck ovcli.conf: <path>OPENVIKING_CLI_CONFIG_FILEenv var overrides the config path hint